Detect if mention panel goes beyond window borders #4266
Conversation
|
Rebased onto latest |
|
Rebased onto latest |
|
I'm not entirely sure about the correctness of the unit test. I think it can be improved 🤔 |
|
It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days. |
github-actions
bot
added
the
pr:stale 🗿
|
Rebased onto latest |
hub33k
force-pushed
the
t/3582
branch
2 times, most recently
from
ce7ac74
to
f376f37
Compare
|
I have doubts about unit tests here. Do you have any suggestions what should be proper way to handle them? 🤔 |
f1ames
removed
the
pr:stale 🗿
|
Rebased onto latest |
|
Strange, it seems adding a comment to a previous review comment, posts pending review comments and commits the review 🤔 🤔 🤔 I'm still reviewing this PR, will post update when ready. |
There was a problem hiding this comment.
The code
Generally looks good 👍
I see that we are not able to cover and test RTL left placement case due to lack of autocomplete RTL support so let's leave it for #4315. But let's leave manual tests in the current state too so we will be able to reuse them when working on it.
It seems that top placement case is not covered too. It is kind of an edge case but can happen with longer editor content:
Manual tests
Very nice idea with manual tests and buttons which changes editors placement 👍 Only some wording suggestions here and there.
Thanks to these tests I have discovered that autocomplete/mentions doesn't support RTL in fact... as already mentioned above, extracted to #4315.
Unit tests
Strangely, some unit tests fails when run from dashboard (http://localhost:1030/#tests/is:unit,path:/tests/plugins/autocomplete) but passes when run separately. Probably something wrong with positioning/sizing stubbing 🤔 Tested on Chrome:
Also you have introduced rect.right which is passed to getViewPositon() so now all unit tests stubbing the rect should use it too.
As for approach to unit tests it looks fine. We need to stub things here since there is no other reasonable solution TBH. If you decide to switch to getViewPaneSize() it should be even easier to stub using sinon.
|
Rebased onto the latest |
What is the purpose of this pull request?
New feature
Does your PR contain necessary tests?
All patches that change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.
This PR contains
Did you follow the CKEditor 4 code style guide?
Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.
What is the proposed changelog entry for this pull request?
What changes did you make?
When mention panel is about to goes beyond window borders it will open in proper position.
Which issues does your PR resolve?
Closes #3582.